refactor(common-adapters): give each popup mode its own module - #29609
Open
chrisnojima wants to merge 2 commits into
Open
chrisnojima wants to merge 2 commits into
chrisnojima wants to merge 2 commits into
Conversation
Popup was one 255-line dispatcher behind a 16-field interface that
branched on runtime state into four unrelated presentations, each
reading a different subset of the props and silently ignoring the rest.
The platform rule was encoded independently in three places.
Split the presentations into three modules with narrow interfaces -
AnchoredPopup, Sheet, ModalCover - and keep Popup as a policy module
that picks one from an explicit intent ('menu' or 'dialog'). The
platform default now lives only there, so usePopup2 no longer gates the
anchor ref and min-writer-role no longer gates its ref either. The type
system rejects placement props on a mode that ignores them, and
hideKeyboard now exists only on the mode that implements it.
Popup no longer takes visible: every caller passed a literal true.
FloatingMenu was the exception - it let mode='modal' past its own
visibility guard and leaned on Popup to drop the hidden menu on the way
past, so that guard is now unconditional.
Sheet and ModalCover get file suffixes, so @gorhom/bottom-sheet and
react-native-screens leave the desktop bundle and the raw div leaves the
native one. ModalCover, previously untestable inside the dispatcher,
gets tests for its press and escape handling.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015rccpV5nLxxC5opF5xzrz7
chrisnojima
force-pushed
the
nojima/HOTPOT-arch-05-popup-modes
branch
from
September 25, 2026 22:04
d71d720 to
633433b
Compare
chrisnojima
marked this pull request as ready for review
September 25, 2026 23:11
Unanchored desktop Toast falls back to ModalCover as it did via Popup; modal FloatingMenu ignores visible again. AnchoredPopup now requires attachTo, Popup spreads its menu props, native FloatingBox dismisses the keyboard on mount, and the unused default exports are gone.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
PopupPropshad 16 fields, andpopup/index.tsxdispatched on runtime state into four presentation modes. Most props were dead in any given mode:PopupCenteredPopupSheetDesktopPopupPositionedThe mode was selected by which combination you passed, so a caller had to already know the platform to fill the interface in correctly. 81 files render
Popup/FloatingMenu; 26 branch on platform within ±15 lines of it.reaction-tooltip.tsxrenders twoKb.Popupelements in one component sharing only two props.The platform rule was encoded three times independently. Changing one default inside
Popuppreviously required editing 13 files to opt out.Change
Three real modules with narrow interfaces —
AnchoredPopup,Sheet,ModalCover— plusPopupkept as a small policy module picking a mode from intent, so the platform default lives in exactly one place and the 40+ menu one-liners are unchanged.The intent set is
'menu' | 'dialog', derived by classifying all 22 real call sites, not invented. Sites whose behaviour never depended on platform now take a mode directly.popup/index.tsx: 255 → 45 lines. The file-suffix seam is restored, so@gorhom/bottom-sheetandreact-native-screenslive only insheet.native.tsx, and the raw<div>only inmodal-cover.desktop.tsx— which also resolves a no-DOM-in-plain-.tsxviolation.Removed as provably dead
visible,mobileAnchored(2 sites →AnchoredPopup), andhideKeyboard's latent trap — it now exists only on the one mode that implements it.usePopup2'sisMobile ? undefined : popupAnchorandmin-writer-role'sref={isMobile ? null : popupAnchor}are both gone.Behavior kept from master
visible—FloatingMenualready returned null for hidden non-modal menus before reachingPopup, andmode="modal"returned its contents directly, never reachingPopupat all. The guard staysif (!visible && mode !== 'modal'), so a modal menu still renders regardless ofvisible. Pinned by tests for both halves.Toast— on master it fell throughPopupto the centered cover.Toastnow rendersModalCoveritself when it has no anchor (only the kick-out confirm's "Kicked" toast); anchoring it would have given the positioner nothing to measure and rendered an invisible box.Contract tightened
AnchoredPopup.attachTois required, so an anchored popup with no anchor no longer type-checks.Popup's'menu'intent keeps it optional and falls back toModalCoverwhen it's missing.Fixed along the way
hideKeyboardnever fired for its one caller (the role picker): nativeFloatingBoxonly dismissed the keyboard when the prop changed, and the caller mounts withtrue. It now dismisses on mount too.Validation
lint:allclean —0 bailed out, 0 whole-props deps, tsc clean both projects.yarn test:unit— 255 suites / 2579 tests green.ModalCovergets 8 tests andFloatingMenu3 — modes that could not be tested at all while they were branches inside a dispatcher importingFullWindowOverlayat module scope.